-
Notifications
You must be signed in to change notification settings - Fork 18
Address couples of feedback. Issue: #26, #36, #37, #38 #45
Address couples of feedback. Issue: #26, #36, #37, #38 #45
Conversation
…nt right at 30m ago
> | ||
<EuiFlexGroup | ||
style={{ padding: '0px 10px', ...props.titleContainerStyles }} | ||
style={{ padding: '0px 20px', ...props.titleContainerStyles }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we change this padding as 0
style={{ padding: '0px', ...props.titleContainerStyles }}
And set the padding of EuiPanel
as 20px
, so we can align title and content body.
<EuiPanel
style={{ padding: '20px', ...props.panelStyles }}
className={props.contentPanelClassName}
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's follow the UX mock up
@@ -105,7 +107,7 @@ const ContentPanel = (props: ContentPanelProps) => ( | |||
className={props.horizontalRuleClassName} | |||
/> | |||
)} | |||
<div style={{ padding: '0px 10px', ...props.bodyStyles }}> | |||
<div style={{ padding: '0px 20px', ...props.bodyStyles }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why increase left and right padding? Shouldn't we increase the top and down padding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, since we cannot increase EuiPanel padding, we have to increase the left/right padding of body
to 20px.
Regarding the top/down padding, you are right, we should make them both 20px. I didn't increase them because:
-
Top padding: I set top padding in AnomaliesLiveChart and AnomaliesDistributionChart:
FlexGroup
has default padding -12px, andEuiHorizontalRule
has default top/bottom margin 8px, changing top padding of FlexGroup to 0px can make the distance betweenEuiHorizontalRule
and FlexItems in FlexGroup to 20px -
Bottom padding: EuiPanel has default padding 16px, thus I keep it for the bottom padding.
To consolidate style changes for ContentPanel, I think I can make following changes so that all the style change can be configured in ContentPanel, and the distance between components and Panel edge or EuiHorizontalRule
is 20px.
<EuiPanel
style={{
paddingLeft: '0px',
paddingRight: '0px',
paddingTop: '20px',
paddingBottom: '20px',
...props.panelStyles,
}}
className={props.contentPanelClassName}
>
<EuiFlexGroup
style={{ padding: '0px 20px', ...props.titleContainerStyles }}
justifyContent="spaceBetween"
alignItems="center"
>
<div
style={{
paddingTop: '12px', // 12px here is because EuiHorizontalRule has bottom margin 8px
paddingLeft: '20px',
paddingRight: '20px',
...props.bodyStyles,
}}
>
{props.children}
</div>
Please let me know your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok about it, we can tune if any other comments come in.
…ddress some comments
// `id` for placeholder data point introduced by `prepareVisualizedAnomalies` shows as legend, | ||
// When there exists anomalies with anomaly grade > 0 | ||
// we make `id` to blank string to hide the legend of placeholder data point | ||
id={!isEmpty(liveAnomalyData) ? '' : ' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious: only blank string can hide legend ? Should we set a normal id rather than whitespace when the liveAnomalyData
not empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very tricky.
If detectorName value for placeholder data is set as null
, the id
of BarSeries
will be shown as legend for placeholder data.
Given empty liveAnomalyData
, the legend of placeholder data will be hidden by changing showLegend
to false, aka showLegend={!isEmpty(liveAnomalyData)}
. Thus we don't need to worry about legend of placeholder data in case of empty liveAnomalyData
.
Given non-empty liveAnomalyData
, the legend of placeholder data is shown along with legend of liveAnomalyData
. Legend of placeholder data is not needed now. Changing id
of BarSeries
to blank string can make legend of placeholder data disappear, and normal legend of liveAnomalyData
is still there.
But we cannot always pass blank string {''}
to id
, as in the case of null
detectorName + id
as {''}
, EuiChart will show No data to dsiplay
, which UX wants to avoid. That's why I pass {' '}
/{''}
to id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After second thought, I changed to
[AD_DOC_FIELDS.DETECTOR_NAME]: !isEmpty(liveAnomalyData) ? '' : null,
and keep using
<BarSeries
id={'Detector Anomaly grade'}`
It can also work. will change to that implementation and post another commit.
from: 0, | ||
size: 1, | ||
search: '', | ||
sortDirection: SORT_DIRECTION.DESC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use ASC sort direction? name
is just string, is it better to show alphabetically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just used to check if there is any detector ever created. That is why size
is 1. If no detector, we move to EmptyDashboard page. If yes, we navigate to the real Dashboard page. using ASC or DESC doesn't matter too much here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any smarter way of doing the check of existing detector is appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, got it, that's ok. Maybe we can use _count
in future.
https://stackoverflow.com/questions/25739888/counting-number-of-documents-using-elasticsearch
Probably _count is a bit faster since it doesn't have to execute a full query with ranking and result fetching and can simply return the size.
Issue #, if available:
Address couples of feedback. Issue: #26, #36, #37, #38
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.